Fix filterByScope to expand lifecycle scopes for build ordering by gnodet · Pull Request #12680 · apache/maven

gnodet · 2026-08-05T03:26:44Z

Summary

The concurrent builder's filterByScope used exact string matching between the lifecycle dependency scope (e.g. "compile") and the declared artifact scope in the model. This meant a provided-scope reactor dependency was not ordered before the consumer's compile phase, since "compile" != "provided".

Root Cause

The lifecycle declares dependencies("compile", READY) for the compile phase, meaning: wait for dependencies with scope "compile" to reach the ready phase before starting compilation. But filterByScope compared this scope string directly against each dependency's declared scope ("provided", "system", etc.), missing all non-compile scopes that still contribute to the compile classpath.

Fix

Expand the lifecycle scope to match all artifact scopes that contribute to it for build ordering:

"compile" → compile, provided, system (+ null defaults to compile)
"runtime" → compile, runtime (+ null)
"test" → all scopes
"test-only" → test only

This is implemented as a private expandScope() method directly in BuildPlanExecutor, keeping the mapping local to where it's needed.

Testing

Updated BuildPlanCreatorTest.testFilterByScopeMatchesExact to cover the expanded scope matching.

gnodet

Well-targeted fix for real concurrency issues in the concurrent builder. The scope expansion fix for provided-scope reactor dependencies and the multi-layer artifact resolution fallback are solid. Three observations after verification (2 FPs filtered out):

Duplicated output directory fallback — The pattern of checking artifact.getFile() for null → creating File(getOutputDirectory()) → testing isDirectory() appears identically at two locations (lines ~139-145 and ~169-173). Consider extracting a resolveReactorProjectFile(MavenProject) helper to avoid keeping both copies in sync.
GAV fallback ignores classifier/type — ArtifactUtils.key() produces groupId:artifactId:version only. If a reactor project publishes both a main JAR and a tests-jar (classifier="tests"), the tests-jar dependency would match the main project and receive target/classes instead of target/test-classes. This is arguably better than null (which is the alternative), but worth documenting as a known limitation in the comment.
Missing test coverage for expandScope() — The test covers "compile", "runtime", and "test" scope expansion but not "compile+runtime", "runtime+system", "test-only", or the default case.

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

Claude Code on behalf of Guillaume Nodet

gnodet · 2026-09-02T19:30:01Z

+                    MavenProject reactorProject = reactorGavMap.get(gavKey);
+                    if (reactorProject != null) {
+                        File fallback = reactorProject.getArtifact().getFile();
+                        if (fallback == null) {


Low: This output-directory fallback block duplicates the identical logic at lines 139-145 above. Consider extracting a shared helper (e.g. resolveReactorProjectFile(MavenProject)) to avoid maintaining two copies.

Also note: ArtifactUtils.key() produces groupId:artifactId:version only — a tests-jar dependency (classifier="tests") would match the main project and receive target/classes instead of target/test-classes. Worth documenting as a known limitation in the "handles type/classifier mismatches" comment above.

Fixed in aa5af67 — extracted resolveReactorProjectFile(MavenProject) helper and documented the GAV-only matching limitation (classifier/type ignored) in the fallback comment.

gnodet

Delta re-review — All three findings from the previous review were addressed by commit aa5af679:

✅ Duplicated output directory fallback → Extracted resolveReactorProjectFile(MavenProject) helper with clear Javadoc. Both call sites now use the shared method.
✅ GAV fallback classifier/type limitation → Comment rewritten to explicitly document the known limitation (tests-jar matching the main project is better than null/build failure).
✅ Missing expandScope() test coverage → Test expanded to cover all six branches (compile+runtime, runtime+system, test-only, default/unknown) with correct assertions.

Delta commit is tightly scoped to exactly the three review findings. LGTM.

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

Claude Code on behalf of Guillaume Nodet

gnodet

Delta re-review — reviewing 4 new commits since the last review (cc6ecd6388, b840c0d896, e1d21ad32d, b73c380d06).

Commit-by-commit assessment

cc6ecd6388 — Remove redundant ThreadLocal, fix after: step semantics* ✅

Correct. session.getCurrentProject() is a ThreadLocal (confirmed in MavenSession.java:65) and attachToThread() calls session.setCurrentProject(step.project) before mojo execution. The previous THREAD_PROJECT ThreadLocal in the concurrent MojoExecutor was therefore redundant and correctly removed. The after:* semantic fix (checking BEFORE + phaseName instead of the phase step itself) is also right: before:compile is the proper entry signal since the phase step itself may have zero mojos and never transition to EXECUTED.

The addLifecyclePhase() belt-and-suspenders call that remains is valid: PhaseRecorder.observeExecution() only records a phase when mojoExecution.getLifecyclePhase() != null, so a step whose mojos don't declare a lifecycle phase would not be recorded by the PhaseRecorder alone. The explicit call covers that gap.

b840c0d896 — Deduplicate scope expansion, promote toScopes to public static — one issue (see inline)

e1d21ad32d — Remove SOURCES→RESOURCES sequential ordering constraint ✅

Clean revert. The constraint is properly moved to plugin-level @After declarations in V4-native plugins.

b73c380d06 — Revert session.getCurrentProject() change in MojoExecutor ✅

Correct — the previous commit already proved getCurrentProject() is ThreadLocal. Revert is consistent.

One finding on toScopes() Javadoc accuracy (see inline comment).

This review was generated by an AI agent, Hermès on behalf of @gnodet.

gnodet · 2026-09-07T17:07:19Z

+     * in dependency resolution for that scope.
+     *
+     * @param classpath the scope identifier (e.g. "compile", "runtime", "test"), may be {@code null}
+     * @return an unmodifiable set of matching artifact scopes, empty if {@code classpath} is null/empty/unknown


Low — Javadoc @return is inaccurate for the default case. The doc says "empty if {@code classpath} is null/empty/unknown" but the implementation returns Set.of(classpath) for unknown values (the default branch). This matters for filterByScope() callers: a plugin-declared lifecycle dependency with an unrecognized scope string (e.g. "provided" passed directly as a scope identifier) will now produce a singleton {"provided"} set instead of empty — a behavior change from the old code. The behavior is probably correct (pass the scope through and let the filter match), but the doc actively misleads.

Suggested change

* @return an unmodifiable set of matching artifact scopes, empty if {@code classpath} is null/empty/unknown

* @return an unmodifiable set of matching artifact scopes; for unknown/unrecognized scope identifiers,

* returns a singleton set containing the scope itself; empty only if {@code classpath} is null or empty

The concurrent builder's filterByScope used exact string matching between the lifecycle dependency scope (e.g. "compile") and the declared artifact scope. This meant a provided-scope reactor dependency was not ordered before the consumer's compile phase, since "compile" != "provided". Expand the lifecycle scope to match all artifact scopes that contribute to it: - "compile" -> compile, provided, system (+ null defaults to compile) - "runtime" -> compile, runtime (+ null) - "test" -> all scopes - "test-only" -> test only This ensures provided/system-scope reactor projects are built before downstream modules that need them for compilation.

slachiewicz mentioned this pull request Aug 8, 2026

Concurrent builder reports an independent, successfully built module as skipped when the reactor halts #12704

Closed

gnodet added this to the 4.1.0 milestone Aug 23, 2026

gnodet added backport-to-4.0.x bug Something isn't working mvn4 labels Aug 23, 2026

gnodet marked this pull request as ready for review August 31, 2026 13:52

gnodet marked this pull request as draft August 31, 2026 13:52

gnodet marked this pull request as ready for review September 2, 2026 18:50

gnodet commented Sep 2, 2026

View reviewed changes

gnodet added a commit to gnodet/maven that referenced this pull request Sep 2, 2026

Update state.json after reviewing PR apache#12680

4c9f8ab

gnodet commented Sep 2, 2026

View reviewed changes

gnodet added a commit to gnodet/maven that referenced this pull request Sep 2, 2026

Update state.json after delta re-review of PR apache#12680

e935fb5

gnodet requested review from cstamas, rmannibucau, slachiewicz and slawekjaranowski September 3, 2026 05:58

slachiewicz removed their request for review September 6, 2026 19:55

gnodet commented Sep 7, 2026

View reviewed changes

rmannibucau approved these changes Sep 7, 2026

View reviewed changes

gnodet force-pushed the run-maven-4-0-x-on-a-few-projects-let-s-start-wit branch from b73c380 to 7058b75 Compare September 7, 2026 18:56

gnodet changed the title ~~Fix concurrent builder race conditions in artifact resolution and phase ordering~~ Fix filterByScope to expand lifecycle scopes for build ordering Sep 7, 2026

gnodet merged commit 7f5d22e into master Sep 7, 2026
24 checks passed

gnodet deleted the run-maven-4-0-x-on-a-few-projects-let-s-start-wit branch September 7, 2026 19:37

github-actions Bot assigned gnodet Sep 7, 2026

gnodet mentioned this pull request Sep 7, 2026

[Backport 4.0.x] Fix filterByScope to expand lifecycle scopes for build ordering #13070

Merged

Navigation Menu

Uh oh!

Fix filterByScope to expand lifecycle scopes for build ordering - #12680

Fix filterByScope to expand lifecycle scopes for build ordering#12680
gnodet merged 1 commit into
masterfrom
run-maven-4-0-x-on-a-few-projects-let-s-start-wit

gnodet commented Aug 5, 2026 •

edited

Loading

Uh oh!

gnodet left a comment

Uh oh!

gnodet Sep 2, 2026

Uh oh!

gnodet Sep 2, 2026

Uh oh!

gnodet left a comment

Uh oh!

gnodet left a comment

Uh oh!

gnodet Sep 7, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	* @return an unmodifiable set of matching artifact scopes, empty if {@code classpath} is null/empty/unknown
	* @return an unmodifiable set of matching artifact scopes; for unknown/unrecognized scope identifiers,
	* returns a singleton set containing the scope itself; empty only if {@code classpath} is null or empty

Uh oh!

Conversation

gnodet commented Aug 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Root Cause

Fix

Testing

Uh oh!

gnodet left a comment

Choose a reason for hiding this comment

Uh oh!

gnodet Sep 2, 2026

Choose a reason for hiding this comment

Uh oh!

gnodet Sep 2, 2026

Choose a reason for hiding this comment

Uh oh!

gnodet left a comment

Choose a reason for hiding this comment

Uh oh!

gnodet left a comment

Choose a reason for hiding this comment

Commit-by-commit assessment

Uh oh!

gnodet Sep 7, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

gnodet commented Aug 5, 2026 •

edited

Loading